Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[QNN] Concat - Refactoring to C++ #3819

Merged
merged 1 commit into from
Aug 31, 2019
Merged

[QNN] Concat - Refactoring to C++ #3819

merged 1 commit into from
Aug 31, 2019

Conversation

anijain2305
Copy link
Contributor

@anijain2305 anijain2305 commented Aug 22, 2019 •
edited
Loading

There are 2 reasons for moving to C++

  • With python interface earlier, we had to call infer_type in the op python definition. This broke the nice abstraction that we have with Relay and QNN ops.
  • Working with different targets, I realized that we need to write QNN passes, that will work on the abstraction of QNN ops. For example, Intel needs the conv to be uint8 x int8. However, TFLite graphs can be uint8 x uint8. This can resolved by writing a legalize pass for QNN conv2d op for Intel machines, that will insert a requantize on the weight matrix to go from uint8 x int8. With earlier python interface, the ops were already getting lowered to relay ops, before I could even run this pass.

Please review @yzhliu @jackwish @FrozenGene @vinx13 @tqchen @zhiics

@zhiics zhiics mentioned this pull request Aug 23, 2019
Merged
@anijain2305 anijain2305 force-pushed the qnn_concat branch 6 times, most recently from ea98355 to 5679038 Compare August 30, 2019 17:21
@anijain2305
Copy link
Contributor Author

@vinx13 @zhiics @yzhliu Can you please review this?

Refactoring to C++ removes InferType dependency. Relevant comment - #3730 (comment)

vinx13
vinx13 requested changes Aug 30, 2019
View reviewed changes
src/relay/pass/pattern_util.h Outdated
@@ -415,6 +415,13 @@ static inline Expr Full(Expr fill_value,
return CallNode::make(op, {fill_value}, Attrs(attrs), {});
}

static inline Expr Concatenate(Expr data, int axis) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is actually the same as MakeConcatenate below isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Nice observation. Made the changes to use that.

src/relay/qnn/util.h Show resolved Hide resolved
zhiics
zhiics requested changes Aug 30, 2019
View reviewed changes
src/relay/qnn/op/concatenate.cc Outdated
Array<tvm::Expr> input_zero_points, double output_scale,
int32_t output_zero_point, int axis) {
auto attrs = make_node<QnnConcatenateAttrs>();
attrs->input_scales = input_scales;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
attrs->input_scales = input_scales;
attrs->input_scales = std::move(input_scales);

src/relay/qnn/op/concatenate.cc Outdated
int32_t output_zero_point, int axis) {
auto attrs = make_node<QnnConcatenateAttrs>();
attrs->input_scales = input_scales;
attrs->input_zero_points = input_zero_points;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
attrs->input_zero_points = input_zero_points;
attrs->input_zero_points = std::move(input_zero_points);

src/relay/qnn/op/concatenate.cc Outdated

/*
* \brief Canonicalizes the QNN concatenate op.
* \param ref_call The original call that will be lowered.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Names in comment mismatch with the arguments.

src/relay/qnn/util.h Outdated
const Array<IndexExpr>& input_shape, const DataType& out_dtype);

static inline Expr Requantize(const Expr& data, const Array<IndexExpr>& input_shape,
const double& input_scale, const int32_t& input_zero_point,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to have const xx& for built-in types, just double input_scale, int32_t input_zero_point etc.

@anijain2305
Copy link
Contributor Author

Thanks @zhiics and @vinx13 for the comments. The changes are in.

zhiics
zhiics approved these changes Aug 31, 2019
View reviewed changes
Copy link
Member

@zhiics zhiics left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@zhiics zhiics merged commit ec7790e into apache:master Aug 31, 2019
@zhenhuaw-me zhenhuaw-me mentioned this pull request Sep 2, 2019
Merged
wweic pushed a commit to wweic/tvm that referenced this pull request Sep 16, 2019
@anijain2305 @wweic
[QNN] Concat - Refactoring to C++ (apache#3819)
f286483
wweic pushed a commit to wweic/tvm that referenced this pull request Sep 16, 2019
@anijain2305 @wweic
[QNN] Concat - Refactoring to C++ (apache#3819)
54b1ae3
wweic pushed a commit to neo-ai/tvm that referenced this pull request Sep 16, 2019
@anijain2305 @wweic
[QNN] Concat - Refactoring to C++ (apache#3819)
11d62df
@tqchen tqchen mentioned this pull request Nov 8, 2019
Closed
@anijain2305 anijain2305 deleted the qnn_concat branch November 13, 2019 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vinx13 vinx13 vinx13 approved these changes

@zhiics zhiics zhiics approved these changes

Assignees
No one assigned
Labels
status: accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@anijain2305 @zhiics @vinx13